Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra socket options: SO_ACCEPTCONN, TCP_KEEPCNT, TCP_KEEPIDLE, TCP_KEEPINTVL #830

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

badeend
Copy link
Contributor

@badeend badeend commented Sep 17, 2023

Add getters and setters for:

  • SO_ACCEPTCONN
  • TCP_KEEPCNT
  • TCP_KEEPIDLE
  • TCP_KEEPINTVL

Also, add getter for SO_REUSEADDR. (Setter already exists)

@badeend badeend force-pushed the extra-socket-options branch from ebf5682 to 687918b Compare September 18, 2023 18:14
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks a lot for adding these!

@sunfishcode sunfishcode merged commit 4358096 into bytecodealliance:main Sep 19, 2023
44 checks passed
@sunfishcode
Copy link
Member

@badeend We don't have illumos in CI, but I do occasionally test rustix on illumos, but with this PR, on illumos, sockopt::test_sockets_ipv4 (but not sockopt::test_sockets_ipv6) now fails with this error:

thread 'sockopt::test_sockopts_ipv4' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', tests/net/sockopt.rs:167:78

At a quick search, I found this illumos bug report which on the surface looks similar.

It's not formally necessary for wasi-sockets to support illumos, but it is worth considering whether there's something we should do there.

@sunfishcode
Copy link
Member

In case it's relevant, the illumos version is openindiana 2023.05.

@badeend
Copy link
Contributor Author

badeend commented Sep 19, 2023

That bug report and the source code seem to suggest that the order in which the options are set matters. I haven't been able to decipher the illumos source, but the bug report mentions that set_tcp_keepintvl before set_tcp_keepcnt should work (?)

I don't have an illumos install, so I can't test it easily but do these changes work for you?: badeend@771f027 If not, how about when set_tcp_keepintvl and set_tcp_keepcnt are swapped around?


It's not formally necessary for wasi-sockets to support illumos, but it is worth considering whether there's something we should do there.

Couple of options:

  1. Nothing. Let it fail (semi-arbitrarily) like any other application already running on illumos.
  2. Let the set_tcp_keepintvl and set_tcp_keepcnt WASI equivalents return not-supported on illumos
  3. (If the order is indeed the culprit) Make a custom provision in the WASI implementation for illumos that defers making the actual syscalls until just before connect (for example)

@sunfishcode
Copy link
Member

I did some experimenting, and it seems to be related to the integer value. A value of 60 works, but the test has 61 which fails.

I think option 1 may be fine here. This seems sufficiently minor that we don't need to make any changes at this time.

@sunfishcode
Copy link
Member

This is now released in rustix 0.38.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants